Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: ensure Multiaddrs are /p2p terminated where possible #4596

Merged
merged 4 commits into from
Nov 14, 2023
Merged

fix: ensure Multiaddrs are /p2p terminated where possible #4596

merged 4 commits into from
Nov 14, 2023
+73 −48

Conversation

stormshield-frb
Copy link
Contributor

@stormshield-frb stormshield-frb commented Oct 5, 2023 •
edited
Loading

Description

Fixes #4573.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

thomaseizinger
thomaseizinger reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

All of these changes need changelog entries please.

swarm/src/lib.rs Outdated Show resolved Hide resolved
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch from 14fecdc to d747cfd Compare October 12, 2023 09:22
@stormshield-frb stormshield-frb mentioned this pull request Oct 12, 2023
Merged
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch from d747cfd to 59a2d57 Compare October 26, 2023 14:45
stormshield-frb
stormshield-frb commented Oct 26, 2023
View reviewed changes
swarm/CHANGELOG.md Outdated Show resolved Hide resolved
mxinden
mxinden reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment in regards to sanitization in libp2p-kad. Overall this turned out very clean. Thank you!

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/kad/src/protocol.rs Outdated Show resolved Hide resolved
swarm/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/identify/src/behaviour.rs Show resolved Hide resolved
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch 3 times, most recently from 5eb013f to 65b76ca Compare October 27, 2023 15:15
thomaseizinger
thomaseizinger reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
@mxinden mxinden mentioned this pull request Oct 28, 2023
Merged
mxinden
mxinden reviewed Oct 28, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, two minor suggestions.

Thanks for upstreaming a nice solution to multiaddr :)

swarm/src/lib.rs Outdated Show resolved Hide resolved
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch 4 times, most recently from 7ca8502 to 858936a Compare October 30, 2023 10:15
@stormshield-frb stormshield-frb marked this pull request as ready for review October 30, 2023 10:15
@mergify
Copy link
Contributor

mergify bot commented Oct 31, 2023

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

@thomaseizinger thomaseizinger changed the title fix(multiaddr): handle multiaddr mismatch with/without p2p proto fix: handle multiaddr mismatch with/without p2p proto Oct 31, 2023
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch 2 times, most recently from faecd23 to d0e5531 Compare October 31, 2023 08:39
@mergify
Copy link
Contributor

mergify bot commented Oct 31, 2023

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch 2 times, most recently from d1d2888 to 379b538 Compare November 2, 2023 08:36
@thomaseizinger
Copy link
Contributor

@stormshield-frb Sorry that this is taking so long. We are currently prioritizing getting the v0.53 release out the door. Once that is done, we will focus on smaller PRs like that can be released as patch-releases.

@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch from 379b538 to 0bdc927 Compare November 2, 2023 12:55
@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 2, 2023

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch from 0bdc927 to a766dbc Compare November 7, 2023 10:00
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch 2 times, most recently from 325b7c5 to 7d0a576 Compare November 7, 2023 10:13
@mxinden
Copy link
Member

mxinden commented Nov 8, 2023

In the meantime, I don't think there is any need for forcepushing the pull request. Once I released rust-multiaddr, I will merge master here.

@stormshield-frb
Copy link
Contributor Author

In the meantime, I don't think there is any need for forcepushing the pull request. Once I released rust-multiaddr, I will merge master here.

Ok sure 😊 I just did it once more because with the release of 0.53 all the CHANGELOG.md where in conflict.

@mergify Mergify

This comment was marked as resolved.

Sign in to view
1 similar comment
@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 10, 2023

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch from 2145b20 to a217468 Compare November 13, 2023 08:33
@stormshield-frb stormshield-frb force-pushed the multiaddr-mismatch-p2p-proto branch 2 times, most recently from 1974003 to 3bf6c1e Compare November 13, 2023 13:16
@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 13, 2023

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

thomaseizinger
thomaseizinger approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thank you! :)

@thomaseizinger thomaseizinger changed the title fix: handle multiaddr mismatch with/without p2p proto fix: ensure Multiaddrs are /p2p terminated where possible Nov 14, 2023
@thomaseizinger thomaseizinger added send-it internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. and removed send-it labels Nov 14, 2023
@mergify mergify bot merged commit d649070 into libp2p:master Nov 14, 2023
71 checks passed
@nazar-pc nazar-pc mentioned this pull request Nov 18, 2023
Merged
1 task
@stormshield-frb stormshield-frb deleted the multiaddr-mismatch-p2p-proto branch November 27, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mismatch with the usage of fully and non-fully qualified multiaddr (/p2p suffix)
3 participants
@stormshield-frb @thomaseizinger @mxinden